Skip to content

feat: add C implementation for math/base/special/rempio2 #2279

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 29 commits into from
Jun 9, 2024
Merged

feat: add C implementation for math/base/special/rempio2 #2279

merged 29 commits into from
Jun 9, 2024

Conversation

gunjjoshi
Copy link
Member

@gunjjoshi gunjjoshi commented May 31, 2024 •
edited
Loading

Description

What is the purpose of this pull request?

This pull request:

Related Issues

Does this pull request have any related issues?

This pull request:

Questions

Any questions for reviewers of this pull request?

No.

Other

Any other information relevant to this pull request? This may include screenshots, references, and/or implementation notes.

This is a draft PR, to discuss the implementation.

Checklist

Please ensure the following tasks are completed before submitting this pull request.

@stdlib-js/reviewers

@stdlib-bot stdlib-bot added the Math Issue or pull request specific to math functionality. label May 31, 2024
gunjjoshi added 3 commits May 31, 2024 18:07
@gunjjoshi
Create test.native.js
62dd882 
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
@gunjjoshi
Create benchmark.c
8a18eba 
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
@gunjjoshi
Create example.c
bb235e5 
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
@gunjjoshi gunjjoshi marked this pull request as draft May 31, 2024 12:40
@gunjjoshi
Copy link
Member Author

gunjjoshi commented May 31, 2024 •
edited
Loading

I was getting failure in some of the test cases. I have referred to FreeBSD version 12.2.0, as a reference.

rempio2: https://svnweb.freebsd.org/base/release/12.2.0/lib/msun/src/e_rem_pio2.c?revision=367086&view=markup
kernel-rempio2: https://svnweb.freebsd.org/base/release/12.2.0/lib/msun/src/k_rem_pio2.c?revision=367086&view=markup

Specifically, if we can check main.c, addon.c and native.js, that would help us out in completing this.

gunjjoshi added 3 commits May 31, 2024 18:21
@gunjjoshi
Update main.c
12b675f 
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
@gunjjoshi
Update native.js
b20b442 
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
@gunjjoshi
removed const from rempio2Kernel parameters
9a7e62b 
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
gunjjoshi and others added 6 commits May 31, 2024 20:03
@gunjjoshi @Planeshifter
Update lib/node_modules/@stdlib/math/base/special/rempio2/manifest.json
d258476 
Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
@gunjjoshi @Planeshifter
Update lib/node_modules/@stdlib/math/base/special/rempio2/manifest.json
0772434 
Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
@gunjjoshi
Update test.native.js
a6bf83f 
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
@gunjjoshi
Update main.c
f47d9e3 
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
@kgryte
Merge branch 'develop' of https://github.com/stdlib-js/stdlib into pr…
3da67fe 
…/gunjjoshi/2279
@kgryte
fix: refactor addon, update docs, and update tests
0df78ac
@kgryte
Copy link
Member

kgryte commented Jun 7, 2024

@gunjjoshi I refactored the addon (see 0df78ac). Tests now pass; however, results for C and JS differ. Currently, if provided 128.0, JS returns ~3.618e-17 and C returns -1.6380356615247201e-19. You'll want to investigate why this is the case.

@gunjjoshi
Copy link
Member Author

@gunjjoshi I refactored the addon (see 0df78ac). Tests now pass; however, results for C and JS differ. Currently, if provided 128.0, JS returns ~3.618e-17 and C returns -1.6380356615247201e-19. You'll want to investigate why this is the case.

Great, thanks for the help! I'll go through the implementation once more to check everything.

@gunjjoshi
use the same exponent mask as that in javascript implementation
4199357 
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
@gunjjoshi
Copy link
Member Author

@kgryte I was able to track down the problem. This was because in the C implementation, earlier the exponent mask the we used had a value of 0x7ff00000. But in the javascript implementation, we were using 0x7ff.

After the changes, both the implementations give the same result for 128.0, i.e., ~3.618e-17.

Also, the lint error that we currently have is arising due to two similar if conditions, but that is required for our implementation.

@gunjjoshi gunjjoshi marked this pull request as ready for review June 8, 2024 14:17
kgryte
kgryte reviewed Jun 8, 2024
View reviewed changes
@kgryte
Apply suggestions from code review
ff58f1a 
Signed-off-by: Athan <kgryte@gmail.com>
kgryte
kgryte reviewed Jun 8, 2024
View reviewed changes
gunjjoshi added 3 commits June 9, 2024 08:20
@gunjjoshi
declare arrays inside corresponding functions
d0c2b06 
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
@gunjjoshi
add extended comment for kernel rempio2
4f81630 
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
@gunjjoshi
add extended comment for kernel rempio2
fd93a9d 
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
@gunjjoshi
Copy link
Member Author

There is a lint error saying the arrays are uninitialized. So, should we initialize them with zeros at the time of declaration directly, or should we declare them first, then initialize them with zeros using a for loop later?

@kgryte
Copy link
Member

kgryte commented Jun 9, 2024

@gunjjoshi I'd go ahead and initialize with zeros at the time of declaration.

@gunjjoshi
initialize arrays F and Q at the time of initialization
c7cad3e 
Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
@kgryte kgryte added Feature Issue or pull request for adding a new feature. C Issue involves or relates to C. Base Issue or pull requests related to "low-level" functionality oriented toward library consumers. labels Jun 9, 2024
Planeshifter
Planeshifter approved these changes Jun 9, 2024
View reviewed changes
Copy link
Member

@Planeshifter Planeshifter left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's get this in to unblock additional C implementation work. Will merge once CI has passed.

Thanks, @gunjjoshi, for your work on it!

@Planeshifter Planeshifter merged commit 0b633eb into stdlib-js:develop Jun 9, 2024
8 checks passed
@gunjjoshi
Copy link
Member Author

Thanks @Planeshifter @kgryte for helping in this.

@gunjjoshi gunjjoshi deleted the rempio2 branch June 11, 2024 03:12
aman-095 pushed a commit to aman-095/stdlib that referenced this pull request Jun 11, 2024
@gunjjoshi @Planeshifter
@kgryte @aman-095
feat: add C implementation for math/base/special/rempio2
8ec00e1 
PR-URL: stdlib-js#2279
Ref: stdlib-js#649

---------

Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
Signed-off-by: Athan Reines <kgryte@gmail.com>
Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Co-authored-by: Athan Reines <kgryte@gmail.com>
Reviewed-by: Philipp Burckhardt <pburckhardt@outlook.com> 
Reviewed-by: Athan Reines <kgryte@gmail.com>
aman-095 pushed a commit to aman-095/stdlib that referenced this pull request Jun 13, 2024
@gunjjoshi @Planeshifter
@kgryte @aman-095
feat: add C implementation for math/base/special/rempio2
10adc69 
PR-URL: stdlib-js#2279
Ref: stdlib-js#649

---------

Signed-off-by: GUNJ JOSHI <gunjjoshi8372@gmail.com>
Signed-off-by: Athan Reines <kgryte@gmail.com>
Co-authored-by: Philipp Burckhardt <pburckhardt@outlook.com>
Co-authored-by: Athan Reines <kgryte@gmail.com>
Reviewed-by: Philipp Burckhardt <pburckhardt@outlook.com> 
Reviewed-by: Athan Reines <kgryte@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kgryte kgryte kgryte left review comments

@Planeshifter Planeshifter Planeshifter approved these changes

Assignees
No one assigned
Labels
Base Issue or pull requests related to "low-level" functionality oriented toward library consumers. C Issue involves or relates to C. Feature Issue or pull request for adding a new feature. Math Issue or pull request specific to math functionality.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gunjjoshi @kgryte @Planeshifter @stdlib-bot